Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Data pipelines unsubscribe modal #19334

Merged
merged 37 commits into from
Jan 5, 2024
Merged

Conversation

tiina303
Copy link
Contributor

@tiina303 tiina303 commented Dec 14, 2023

Problem

https://github.com/PostHog/product-internal/pull/533

Added a button for testing it to apps management page, we should remove that before merging

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

Screenshot 2023-12-14 at 16 43 19 Screenshot 2023-12-14 at 16 43 13 Screenshot 2023-12-14 at 16 43 28

=> clicked the button, which opened the modal
Screenshot 2023-12-14 at 16 43 33

=> clicked disable on everything, which made unsubscribe button possible
Screenshot 2023-12-14 at 16 43 48

then saw the lemonToast after hitting unsubscribe

@tiina303 tiina303 requested review from Twixes, raquelmsmith and a team December 14, 2023 17:08
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Dec 14, 2023

Size Change: 0 B

Total Size: 2 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 2 MB

compressed-size-action

@tiina303 tiina303 force-pushed the exports-unsubscribe-modal branch from 56297fe to a90c02f Compare December 15, 2023 11:20
@tiina303 tiina303 marked this pull request as ready for review December 15, 2023 11:20
@tiina303 tiina303 force-pushed the exports-unsubscribe-modal branch from a90c02f to 1c192e5 Compare December 15, 2023 11:20
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@raquelmsmith
Copy link
Member

raquelmsmith commented Dec 22, 2023

I hooked this up to the existing billing modal:

image

(was using group analytics to test this, but it's for Data Pipelines addon)

@raquelmsmith raquelmsmith requested a review from xrdt December 22, 2023 22:39
@raquelmsmith raquelmsmith self-assigned this Dec 22, 2023
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

3 snapshot changes in total. 0 added, 3 modified, 0 deleted:

  • chromium: 0 added, 3 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@tiina303
Copy link
Contributor Author

tiina303 commented Jan 2, 2024

Thanks @raquelmsmith the billing part looks good to me, though are we sure we want to call it 'product analytics + data stack'?

@tiina303 tiina303 removed the stale label Jan 2, 2024
@raquelmsmith
Copy link
Member

though are we sure we want to call it 'product analytics + data stack'?

Are you suggesting we just call it "Product analytics" and remove the "+data stack"? That makes sense, esp with data warehouse coming up. I'll make that change in billing.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@raquelmsmith raquelmsmith requested a review from xrdt January 3, 2024 23:00
@tiina303
Copy link
Contributor Author

tiina303 commented Jan 4, 2024

frontend/snapshots/scenes-app-pipeline--pipeline-transformations-page--dark.png screenshots look broken now - the app name is missing for GeoIP app

@raquelmsmith raquelmsmith enabled auto-merge (squash) January 4, 2024 16:35
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple rough edges in the UI that should be quick to address, but looks alright overall.
Might be useful to go through this flow in person on Monday, just to see it works smoothly @tiina303

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there's only a small layout shift here, but a few notes on this UI:

  1. Pretty weird that the activation CTA ("Create your first transformation") is visible even if there already are transformations
  2. Item icons should always be the first column, and icon columns shouldn't have a name – in this case "App" is the icon column
  3. The "Order" column should have width: 0 so that it takes up as little space as possible

Might be good as a separate PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this UI was touched as part of this PR? But not sure why this snapshot would have changed... either way I'll leave you two to update this in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Team" is our internal naming, UI should only ever say "Project".
Also gonna repeat the point with the icon looking weird if it's not the first column. In this case I think the project can be on the right – what do you think?

@raquelmsmith raquelmsmith disabled auto-merge January 4, 2024 21:17
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@raquelmsmith raquelmsmith merged commit 0c96017 into master Jan 5, 2024
99 checks passed
@raquelmsmith raquelmsmith deleted the exports-unsubscribe-modal branch January 5, 2024 19:20
jacobwgillespie pushed a commit to jacobwgillespie/posthog that referenced this pull request Jan 12, 2024
* feat: Exports unsubscribe modal

* Changes

* Fix query

* Make modal work for plugin configs

* plugins part should work now

* wip batch exports

* batch exports finished + modal access listeners

* fix name and description to be writable

* rename and move table to main billing unsub modal

* add text and disabled reason

* improve disabled state

* Update UI snapshots for `chromium` (1)

* move into billing

* use correct product name

* stories

* fix

* make images smaller

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (1)

* Update frontend/src/scenes/plugins/plugin/PluginImage.tsx

Co-authored-by: Bianca Yang <[email protected]>

* remove old logic

* fix

* fix

* fix overlap

* Update UI snapshots for `chromium` (1)

* delete snapshots so they are regenerated

* Update UI snapshots for `chromium` (1)

* fix test

* unfix?

* project not team

* reorder columns

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (1)

---------

Co-authored-by: Michael Matloka <[email protected]>
Co-authored-by: Raquel Smith <[email protected]>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Bianca Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants